Repository navigation
Conversation
|
as I understand it this is a AI generated fix. I fear I don't want to invest the time to review this. |
|
Can you please respond how the code is written. By hand/by AI? I would like that checkers and other code in cppcheck are written carefully by hand.
I can envision to have AI-generated code in stuff that we don't release for instance AI generated tests. But I would appreciate if you are clear about what is AI generated. |
|
Test results for commit f198ea5 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8893/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I didn't find any new false positive, crash or hang. The PR only removes warnings, and every case I built to trick the overload matching into warning still came out silent, which is the correct result.
The thing the maintainers should weigh is what it costs in missed warnings. In C++, constStatement on a comma now goes away whenever either operand has an unknown type or a class/enum type. This happens even if the class is fully defined in the file and no operator, is declared anywhere. Some examples that warned before and are now silent:
g(), 1;whereS g();returns a plain structx = 1, s;wheresis that structt = 1, 2;whereT& T::operator=(int)exists but there is nooperator,e = A, 1;whereeis an enumunknown(), 1;
The author acknowledges this trade-off in the description. The cost could be reduced by only giving up when a type is unknown or incomplete, or when some visible operator, could take that class or enum. This would still cover the Boost.Assign case, where std::vector<int> has no visible typeScope.
Apart from that, the new matching code fails safe in the cases I tried: template candidates, mixed int/long overloads and unrelated operators all lead to "no warning".
Testing
- Built
cppcheckandtestrunnerinpr/andcppcheckinbase/; both builds succeeded. ./testrunner TestIncompleteStatement TestOther: 285 tests, 0 failures.- Compared base and PR on scratch files with
cppcheck -q --enable=warning t.cpp.
Warnings lost with the PR (base warns Found suspicious operator ',', result is not used. [constStatement], PR prints nothing):
struct S { int x; };
S g();
void f1() { g(), 1; } // base warns, PR silent
enum E { A }; void f2(E e) { e = A, 1; } // base warns, PR silent
struct T { T& operator=(int); };
void f3(T t) { t = 1, 2; } // base warns, PR silent (no operator, anywhere)
void f4(int x, S s) { x = 1, s; } // base warns, PR silent
void f5() { unknown(), 1; } // base warns, PR silentUnchanged results: these still warn in both: a == 1, 2, x = 1, 2, s.x = 1, 2, *p, 0, x++, 1, x += 1, 2 for unsigned char and double, p = &s.x, 2, the ternary duplicateExpressionTernary case, and unusedScopedObject on L(a, 1);.
Overload matching: I tried a template operator+=(D&, T*), a free E2& operator+=(E2&, long) next to a member int operator+=(int), and a template operator,. The PR either suppresses the warning or matches base; it never adds a warning.
Not tested: my run of test/cfg/std.cpp used options that don't match the cfg test setup and produced unrelated unmatchedSuppression output on base too. The only constStatement expectation there (spn[0];) is not a comma expression.
Automated review of f198ea5 by Claude. The pull request was built and tested locally.
| return isConstStatement(tok->astOperand2(), library, platformIndependent); | ||
| return isConstStatement(tok->astOperand2(), settings, platformIndependent); | ||
| if (Token::simpleMatch(tok, ",")) { | ||
| if (tok->isCpp() && (!isBuiltinCommaOperand(tok->astOperand1(), settings) || !isBuiltinCommaOperand(tok->astOperand2(), settings))) |
There was a problem hiding this comment.
AI review
In C++ this drops the warning whenever an operand is not a builtin type. That includes class or enum types that are fully known and have no operator, declared anywhere in the TU. Examples that warned before and are now silent:
struct S { int x; };
S g();
void f1() { g(), 1; }
void f4(int x, S s) { x = 1, s; }
struct T { T& operator=(int); };
void f3(T t) { t = 1, 2; }You could suppress only when the type is unknown or incomplete (no typeScope, as in the Boost std::vector<int> case), or when the symbol database has an operator, member or non-member that could take that class or enum. That would keep these warnings.
|
can you please comment about the AI review. Do you feel AI suggestion is bad? Do you want to fix the lost warnings in this PR? If you feel that the lost warnings should be fixed later in a separate PR then we can create a ticket about it. |
Addresses Trac 4651. Boost.Assign expressions such as
values += 2, 2can perform useful work through an overloaded comma, but currently receiveconstStatementeven in ordinary warning mode.Require evidence of built-in operand types before treating a C++ comma expression as a constant statement. Account for assignment overloads separately: their inferred type is copied from the lhs, while the declared operator may return a different type. Conservatively inspect available return types without selecting an arbitrary overload; unknown, inherited, template and user-defined-conversion cases remain uncertain.
Keep warnings for ordinary scalar, pointer and C comma expressions, plus declared integer/pointer/void operator results. Add three test methods covering the original numeric/string-list use, member and free operators, chained commas, enums, anonymous namespaces, template ADL, mixed overloads and a ref-qualified/user-conversion regression. No Boost-name special case or library configuration change.
Validation:
git diff --checkpass.Two earlier Windows runs exited with
0xc0000005while displayingTestIO::testPrintfArgumentVariables, whose checker does not invoke the new helper. The recorded offsets resolve to the unchangedToken::Match; neither a patch nor baseline cause is established. A bounded debugger run passed that location but was stopped after becoming slow in the existingTestValueFlow::valueFlowHang. The final complete native pass above includes the conversion regression.This is conservative classification, not complete overload resolution: uncertain class expressions can suppress a genuine warning. The primitive/pointer/void controls limit that tradeoff and preserve the demonstrated existing diagnostics.
Please assign Trac 4651 to KiritoYG for this patch and confirm eligibility under the published USD 30 bounty bracket. After qualifying closure, I can use the documented bounty-request process; please also confirm the available settlement channel.